-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try: Fix image responsive rules. #39045
Conversation
Size Change: -35 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
Thanks, Joen, for the quick follow-up. Do you remember if any core themes have manually inserted images? I'm also going to ping @kjellr and @MaggieCabrera. |
I think wp-image- won't affect images like the featured image, but that block already has that rule built-in, so I think this is ok to do this way |
Small comment: Just realized that the proposed selector would only work for images where the first class is the Therefore, it might be better to change the rule to this:
Notice the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, fixed 👍 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me. I'd probably ask for a +1 in case folks are able to spot other edge cases.
Note that :where
doesn't have support in IE, I know we no longer support this browser, but I'm noting this since the CSS here is also used in the published page.
Outside of official support, I'd think it fine to keep as is. It's an extra guard-rail for responsive images, and it's one that many themes keep as part of their reset styles regardless. @Mamaduka should we land this one? It might be good to start with this one, then we can see if we need to employ the revert instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
I have a question, when we create block patterns, we usually remove everything related to the image ID since we don't know what it will be for the particular install of each person that will insert the pattern, and that prevents this rule from applying. What would be the best way to go about in those cases? |
That's a really good question, @MaggieCabrera. I found this pattern which does so:
Here's comparative markup for the Image as manually inserted:
Would you say the above two examples are representative of the problem or are there other markup types we need to account for? If the above is representative, instead of this:
— we could maybe do this:
or we could restore this rule, so a duplicate rule exists for the Image Block specifically? If you think that'd work, I'd happy to make a PR for either. |
Thanks for having a look @jasmussen ! That's exactly the case I was facing myself. I think both solutions work fine and I don't have a preference. It makes sense to me that the rule is on the block I suppose. That case also has less selectors in play. |
* Try: Fix image responsive rules. * Address feedback. Props @nasif-co
Hey, just to add in that I ran into issues with this on a site just now and I think it's relevant to your proposed fix here. The presence of this CSS declaration meant that I was relying on it in a newly-written theme to resize my images to max-width, but that use of the relatively new |
I think of this rule as a progressive enhancement, a very soft fallback for when the theme itself doesn't output these rules. That comes with the downside of older browsers not necessarily supporting it. The problem is, we can't increase the specificity or it has side effects, meaning the main alternative is to remove the fallback and let themes handle this entirely. |
In my case - as a theme developer working on a theme this week - I saw the declaration and said "looks like Wordpress does this for me now, neat". Didn't add my own. And then got broken on older browsers. |
Yep, unfortunate situation. |
Description
Changes the approach to image responsive from #38399, and should fix issues outlined in the comments. Props @nasif-co for the suggestion.
38399 moved a CSS rule intended to help add responsive guardrails to images, so that it would apply across the frontend. This had side effects for images that weren't meant to be responsive.
This PR fixes it by scoping those styles to just images inserted manually in the editor, either block or classic.
See also #39046 for a revert alternative.
Testing Instructions
Insert an image in an editor, observe that it's responsive on the frontend.
Note that images that weren't manually inserted should not be targetted by these rules.
Checklist:
*.native.js
files for terms that need renaming or removal).